Skip to content

bundle/dstate: nullify local state when DMS recording enabled - #6541

Draft
shreyas-goenka wants to merge 46 commits into
mainfrom
isaac/dms-nullify-local-state
Draft

bundle/dstate: nullify local state when DMS recording enabled#6541
shreyas-goenka wants to merge 46 commits into
mainfrom
isaac/dms-nullify-local-state

Conversation

@shreyas-goenka

Copy link
Copy Markdown
Contributor

Summary

Fixes a bug where enabling DMS recording (experimental.record_deployment_history) on a deployment fails when the local state cache still has resources, even if the remote deployment was destroyed. The fix nullifies local state when recording is enabled, treating the state file as a tombstone that only carries the feature marker and header.

The Problem

When a DMS deployment is destroyed on the service, the local resources.json is a cache that can retain stale resources. Attempting to redeploy with DMS enabled would fail with:

Error: this deployment already exists and is not recorded with the deployment history
feature enabled, so it cannot be recorded without redeploying its resources

This error was caused by a guard that checked recording && !recorded && len(db.Data.State) > 0. While the guard was originally designed to prevent enabling DMS on existing non-DMS deployments (creating resources a second time), it also blocked the legitimate case of a destroyed DMS deployment being rebuilt.

The Solution

When opening a deployment state with recording enabled, nullify the local state. The service becomes the source of truth:

  • If dmsDeploymentID is non-empty: ListResources fetches resources from the service
  • If dmsDeploymentID is empty: no resources are loaded (fresh or destroyed deployment)

This allows the bootstrap path to be taken (recording && !recorded && len(db.Data.State) == 0) instead of erroring.

Tradeoff

Nullifying local state removes the guard against enabling DMS on an existing non-DMS deployment (no service node, but resources cached locally). This could:

  • Allow migrating from non-DMS to DMS without explicit destroy
  • Potentially orphan old resources if they have different IDs or names

However, this is an acceptable tradeoff to unblock the more common and important case: recovering destroyed DMS deployments. Users who need to migrate from non-DMS to DMS should explicitly destroy the bundle first.

Changes

  • bundle/direct/dstate/state.go: Nullify local state when recording is true
  • bundle/direct/dstate/state_test.go: Add unit test verifying preconditions
  • acceptance/bundle/dms/existing-state/output.txt: Updated golden (reflecting changed guard behavior)

Testing

  • Unit tests pass: go test ./bundle/direct/dstate/...
  • Existing-state DMS acceptance test now reflects the new behavior (no error when enabling DMS on non-DMS deployment)

This pull request and its description were written by Isaac.

shreyas-goenka and others added 30 commits September 4, 2026 01:55
Record each deploy's resource state to the deployment metadata service and
read it back, making the service the source of truth for a recorded bundle:
the local state file becomes a tombstone and a leftover WAL is discarded.
Gated on experimental.record_deployment_history (direct engine only), and
supports deploy --plan.

Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
…anged

Co-authored-by: Isaac <no-reply@databricks.com>
…source

Co-authored-by: Isaac <no-reply@databricks.com>
…, drop dedicated summary test

Co-authored-by: Isaac <no-reply@databricks.com>
…op redundant operation-upload-message test

Co-authored-by: Isaac <no-reply@databricks.com>
…pen reads the workspace client from context

Co-authored-by: Isaac <no-reply@databricks.com>
…hem from DeploymentBundle

The phases now reach DMS through StateDB (DmsClient, OperationBuffer, TakeOperationBuffer)
rather than DeploymentBundle mirror fields. The operation drain stays before CompleteVersion.

Co-authored-by: Isaac <no-reply@databricks.com>
…ffer in StateDB

- add dstate.StorageBackend (WORKSPACE_FILESYSTEM/DEPLOYMENT_METADATA_SERVICE), set by Open from the state marker; the plan carries its string form so deploy --plan can reject a plan whose target switched backends
- StateDB owns the recording lifecycle: RecordingError() and CompleteVersion() (idempotent via versionCompleted), with the operation-buffer drain moved into Finalize; drop the exposed OperationBuffer/TakeOperationBuffer getters and the recorder() indirection
- gate DMS behavior on StorageBackend()==DEPLOYMENT_METADATA_SERVICE / ConfiguresDeploymentHistory instead of DmsClient()!=nil
- consolidate plan validation in validatePlan (config-vs-plan backend match + stale-plan guard)

Co-authored-by: Isaac <no-reply@databricks.com>
…ate unbind, restore v3 read

- rename StampDeploymentID -> StampDeploymentIdForFirstVersion (only used for the first version's id)
- inline RecordDeploymentHistoryEnv into RecordsDeploymentHistory (single predicate)
- unbind: drop the config-based phase check; DeploymentBundle.Unbind opens without recording, so the state marker (not the config flag) refuses a recorded deployment
- restore forward-compat read of featureStateVersion=3: this CLI still writes v2, but reads a v3 state with no features and refuses one that carries a feature

Co-authored-by: Isaac <no-reply@databricks.com>
Extend declined-deploy, depends-on and deployment-metadata-change to capture the
serialized 'bundle plan -o json' (now carrying storage_backend), the DMS
list-resources view, and the raw recorded state. deployment-metadata-change pins
state_path, which print_state.py cannot resolve, so it relies on list-resources.

Co-authored-by: Isaac <no-reply@databricks.com>
…ate across the suite

Extend no-resources, multiple-resources, record, successful-recreate, no-drift,
stale-plan, emptied-resource and provenance to capture the serialized
'bundle plan -o json' (now carrying storage_backend), the DMS list-resources view,
and the raw recorded state at each steady-state point. no-drift shows only the
updated resource bumping to version 2; successful-recreate shows the recreate at
version 2; provenance masks the git commit.

Co-authored-by: Isaac <no-reply@databricks.com>
…-version golden

- deploy.go: StorageBackend() asserts the state is open, but only the direct engine
  opens it, so gate the DMS deployment-create block on stateEngine.IsDirect() first.
  Fixes a panic ("DeploymentState must be opened first") on terraform deploys, which
  the direct-only dms tests did not exercise.
- future_version: supported state version is now 3 (v3 is read-forward-compatible),
  so the too-new message reports 3.

Co-authored-by: Isaac <no-reply@databricks.com>
…version on recording error

- C11: StateDB holds DeploymentID/LatestVersionID (set at Open from the service); CalculatePlan
  stamps the plan's lineage from StateDB instead of the bundle config tree (config.History stays
  only for 'bundle summary'). Behavior-neutral.
- unbind: Open returns a sentinel ErrUnsettingRecording; DeploymentBundle.Unbind maps it to an
  unbind-specific message instead of the deploy/destroy-flavored guard text.
- CompleteVersion now completes as Failure when the drained buffer holds a recording error, even
  if the caller passed success=true, so a destroy whose uploads failed keeps its deployment record.

Co-authored-by: Isaac <no-reply@databricks.com>
… the readplan option

The dedicated readplan test duplicated stale-plan, which already deploys from a saved raw
plan under recording. Delete it and add the standard READPLAN matrix to stale-plan so its
initial deploy runs both from the saved plan and recomputed - the deploy is untraced so the
variant-dependent command stays out of the golden. Recorded operations still carry the
deployment_id/version_id lineage (asserted in depends-on and others).

Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Extend the failure-injection tests to show how DMS records a failed or refused deploy:
failed-delete/recreate/update, failed-update-permissions, operation-upload-fails-once,
record-failure, version-never-created (and a WORKSPACE_FILESYSTEM plan on existing-state's
first deploy). Each captures the healthy recorded state and the recorded result of the failure.

Co-authored-by: Isaac <no-reply@databricks.com>
… wording

- rename StartRecording -> InitializeOperationBuffer (it opens the buffer; kept separate from Open
  because a first deploy's id does not exist until after approval)
- collapse the duplicated deployment/version fields to one public pair (DeploymentID/LatestVersionID);
  InitializeOperationBuffer records the first-deploy id, CompleteVersion derives the version from it
- RecordingError is now a no-op unless the backend is the deployment metadata service, keeping the
  non-recording path cleanly separate (like RecordFailure)
- stop surfacing the internal "deployment metadata service" name in user-facing errors; say
  "deployment history feature enabled" / "the deployment history service"
- drop the "in place of nil client checks" comment and restore the original featureStateVersion
  wording (minimal C12 diff)

Co-authored-by: Isaac <no-reply@databricks.com>
…ploy test

Three bugs introduced by the previous hand-edit commit:

1. unlockedOpen: the 3-way switch reconciling config vs state was replaced
   with a simple `if !stateHasRecordDeploymentHistory && withDeploymentHistory`,
   which errored even on brand-new deployments (no state file → no feature flag
   → error). Restored the original logic including the bootstrap case for
   `recording && !recorded && len(db.Data.State) == 0`.

2. unlockedOpen: recording opens must delete a leftover WAL (the service owns
   the resources; a local log from a crash is irrelevant). The hand edit changed
   this to "ignore the WAL", breaking version-never-created and existing-state.
   Restored os.Remove on recording opens.

3. Error message indentation: the multi-line error had tab-indented continuation
   lines instead of no leading whitespace, mismatching the existing-state golden.

Also: declined-deploy/script - add --get to the second print_requests call so
the GET requests made during the declined deploy (GetDeployment + ListResources)
appear in the output. Move out.requests.txt cleanup into the EXIT trap so the
catalog delete's request file is removed and no out.requests.txt is left behind.

Co-authored-by: Isaac <no-reply@databricks.com>
Remove | nostamp.py from all bundle/dms test scripts. nostamp was stripping
deployment_id, next_version_id, and last_version_id from plan output (its
purpose is to make non-DMS tests agnostic to recording mode). DMS-specific
tests should assert these fields directly — they are the feature under test.

Regenerate all goldens. Plans now show deployment_id/next_version_id/
last_version_id after the first deploy, confirming the fields are serialized.

Co-authored-by: Isaac <no-reply@databricks.com>
Add --no-dms flag to print_state.py: prints resources.json as-is without
fetching state from the deployment metadata service. The existing behaviour
(enriching the output with DMS-held resources) exists for non-DMS tests so
their goldens stay identical whether recording is on or off; DMS tests need
the opposite — to assert that the local file IS the tombstone (empty state,
just the header + record_deployment_history feature flag).

Update all bundle/dms test scripts to use print_state.py --no-dms.
Regenerate goldens: state blocks that previously showed full resource state
now show "state": {}, confirming the service holds the resources and the
local file carries only the tombstone marker.

Co-authored-by: Isaac <no-reply@databricks.com>
…re-deploy plan in depends-on

Add --no-dms flag to print_state.py: prints resources.json as-is without
fetching state from the deployment metadata service. The existing behaviour
(enriching the output with DMS-held resources) exists for non-DMS tests so
their goldens stay identical whether recording is on or off; DMS tests need
the opposite — to assert that the local file IS the tombstone (empty state,
just the header + record_deployment_history feature flag).

Update all bundle/dms test scripts to use print_state.py --no-dms.
Regenerate goldens: state blocks now show "state": {}, confirming the
service holds the resources and the local file carries only the tombstone.

Also add a bundle plan -o json call before the first deploy in depends-on,
showing that no deployment_id or version fields appear before any deploy exists.

Co-authored-by: Isaac <no-reply@databricks.com>
…te WAL header

The leftover WAL now carries the feature flag, so the test proves the WAL is
discarded rather than replayed: if it were merged the feature would leak into
the refused deploys, but the refusals still stand and the final fresh deploy's
feature comes from config, with resources.json.wal gone (--expect 0).

Co-authored-by: Isaac <no-reply@databricks.com>
…change

These two section-header titles printed to stdout immediately before a command
whose trace line goes to stderr. On cloud the two streams interleave
nondeterministically and the title line is dropped, so the golden never
matched. The surrounding >>> commands and their output already convey the
state, so the titles are removed rather than fixing the shared title() helper
(which would add a blank line after every title in every acceptance golden).

Co-authored-by: Isaac <no-reply@databricks.com>
In DMS/direct mode only resources.json exists among the state files, so the
filename check added nothing; the records_deployment_history() + --no-dms
gate alone selects the recorded-state path.

Co-authored-by: Isaac <no-reply@databricks.com>
shreyas-goenka and others added 16 commits September 7, 2026 02:04
…tions

Add a ListOperations handler to the testserver (GET
/api/2.0/bundle/deployments/{id}/versions/{version}/operations) mirroring the
service, and call bundle-deployments list-operations in failed-delete to assert
the failed delete operation carries its error message and FAILED status.

Co-authored-by: Isaac <no-reply@databricks.com>
…omment

Reverts a comment rewording; the longer original phrasing is clearer about why
a featureStateVersion state with no features is accepted without running the
migrations and its on-disk version left as-is.

Co-authored-by: Isaac <no-reply@databricks.com>
…og (cloud-enabled)

Drop the fault.py injection; point the recreated schema at a catalog that does
not exist so the create half of the recreate fails on its own. The testserver
now models catalog existence: SchemasCreate returns 404 CATALOG_DOES_NOT_EXIST
when the parent catalog is absent, and the fake seeds the "main" catalog so the
common case (schemas in main) keeps working. This makes the failure identical
locally and on a real workspace, so the test runs with Cloud = true.

Co-authored-by: Isaac <no-reply@databricks.com>
…ppingVersion

Re-add the test pinning the featureStateVersion special case: a v3 state with no
features is accepted and left at v3 (not flipped to currentStateVersion), while a
v3 state recording any feature is refused with the upgrade hint. It is the forcing
function for the deferred version bump.

Co-authored-by: Isaac <no-reply@databricks.com>
RecordingError only reads operationBuffer and calls buf.Err() (a quick read on
the buffer's own mutex), so holding db.mu across it via defer is safe and reads
cleaner. CompleteVersion keeps its manual unlock: the network work after the
snapshot must run outside the lock.

Co-authored-by: Isaac <no-reply@databricks.com>
…history

New test showing bind and unbind against a bundle that records deployment
history. Before any deploy there is no recorded state, so bind just fails to
find the workspace resource and unbind is a no-op. Once a deployment is
recorded, both are refused ("bind/unbind is not supported for a bundle that
records deployment history"): the service owns the resource set, so adopting or
releasing a resource by editing local state is not allowed. Local-only, since
the refusal is a client-side gate.

Co-authored-by: Isaac <no-reply@databricks.com>
RecordOperation and RecordFailure each took db.mu separately to touch latestState
and then again inside record to touch pending, so the two maps could be observed
mid-update. Move the latestState read/write into record, guarded by the same lock
as the pending update: a state write (isStateWrite) records the latest state, a
failure merges its status/message onto that recorded state (or falls back to the
passed update). RecordOperation and RecordFailure no longer lock directly.

Co-authored-by: Isaac <no-reply@databricks.com>
…ull -o json

Add a dms_plan helper (bundle/dms/script.prepare) that prints the plan JSON with
the per-resource plan body dropped (jq del(.plan)) — leaving the header fields
cli_version, lineage, serial, deployment_id, next/last_version_id, storage_backend
— then runs bundle plan again for the readable summary. Replace every full
`bundle plan -o json` dump in the bundle/dms tests with it.

The per-resource plan carried remote_state that diverges between the fake and a
real workspace (e.g. a serverless pipeline's effective_serverless_compute_id),
which broke no-drift on cloud; dropping it keeps local and cloud identical while
still asserting the DMS header fields and the human-readable plan.

Co-authored-by: Isaac <no-reply@databricks.com>
…xistingUpdateIfAny

Trim the verbose doc comments to a couple of plain lines, and rename record's flag
from isStateWrite to keepExistingUpdateIfAny (inverting it): true merges a failure
onto the resource's last recorded state if one exists, false saves the update as
that latest state.

Co-authored-by: Isaac <no-reply@databricks.com>
Create a real workspace job and bind to it, so bind gets past fetching the
resource and reaches the config gate that refuses bind while recording
deployment history - recording that error instead of "Job 123 does not exist".

Co-authored-by: Isaac <no-reply@databricks.com>
…nown bug, Cloud=false

Emptying a grants node makes apply call DeleteState, recording a succeeded UPDATE
that names no state while the schema still exists; a real workspace rejects that
(400 INVALID_PARAMETER_VALUE). The fake does not model the check, so the recorded
output is the buggy behavior. Disable the cloud run and document the fix needed
(emptying grants should record an update carrying the emptied state, not a delete).

Co-authored-by: Isaac <no-reply@databricks.com>
…a list-operations

Add a bundle-deployments list-operations call after the failed permissions
update so the golden asserts the failed operation (jobs.foo.permissions, UPDATE,
FAILED) carries the 403 error message the service recorded.

Co-authored-by: Isaac <no-reply@databricks.com>
Dropped as unnecessary; operation-upload-fails-once still covers the
upload-failure path.

Co-authored-by: Isaac <no-reply@databricks.com>
Both duplicate coverage in the remaining bundle/dms tests: recording calls and
serialized operation state are exercised by successful-recreate, multiple-resources,
depends-on and others; a failed update is covered by failed-update-permissions and
failed-recreate. Reword the two comments that pointed at bundle/dms/record.

Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
When opening a deployment state with experimental.record_deployment_history enabled,
the service is the source of truth for resources, not the local state file. Nullify
the local state so it acts as a tombstone (carrying only the feature marker and
header). This allows stale local state from a destroyed DMS deployment to bootstrap
a fresh deployment, instead of erroring with "this deployment already exists and is
not recorded".

The fix removes the guard that previously prevented enabling DMS on deployments with
local resources. This is a necessary tradeoff to support the common scenario where a
DMS deployment is destroyed on the service but the local state cache remains. The
service is now authoritative:
- If dmsDeploymentID is non-empty: ListResources fetches the service's resources
- If dmsDeploymentID is empty: no resources are loaded (fresh/destroyed deployment)

Tradeoff: this also allows enabling DMS on an existing non-DMS deployment, which could
leave old resources orphaned if they have different IDs or names. Users should destroy
a non-DMS deployment before enabling DMS to avoid this scenario. Accept this tradeoff
to unblock the more common case (destroyed DMS bootstrap).

Co-authored-by: Isaac <no-reply@databricks.com>
@eng-dev-ecosystem-bot

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 0336c85

Run: 34074400399

Env ❌​FAIL 🟨​KNOWN 🔄​flaky 🙈​SKIP ✅​pass 🙈​skip Time
❌​ aws linux 2 1 2 1 302 1224 7:26
❌​ aws windows 2 1 1 306 1222 7:49
❌​ azure linux 27 1 1 278 1224 5:35
❌​ azure windows 27 1 1 280 1222 5:31
❌​ gcp linux 27 1 1 279 1224 5:51
❌​ gcp windows 27 1 1 281 1222 5:38
29 interesting tests: 27 FAIL, 1 KNOWN, 1 SKIP
Test Name aws linux aws windows azure linux azure windows gcp linux gcp windows
🟨​ TestAccept 🟨​K 🟨​K 🟨​K 🟨​K 🟨​K 🟨​K
❌​ TestAccept/bundle/destroy/jobs-and-pipeline ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/destroy/jobs-and-pipeline/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/declined-deploy ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/declined-deploy/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/depends-on ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/depends-on/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/deployment-metadata-change ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/deployment-metadata-change/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/existing-state ❌​F ❌​F ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/existing-state/DATABRICKS_BUNDLE_ENGINE=direct/DMS= ❌​F ❌​F ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/failed-recreate ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/failed-recreate/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/multiple-resources ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/multiple-resources/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/no-drift ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/no-drift/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/no-resources ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/no-resources/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/provenance ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/provenance/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/record-failure ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/record-failure/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/stale-plan ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/stale-plan/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true/READPLAN= ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/stale-plan/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true/READPLAN=1 ✅​p ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/successful-recreate 🔄​f ✅​p ❌​F ❌​F ❌​F ❌​F
❌​ TestAccept/bundle/dms/successful-recreate/DATABRICKS_BUNDLE_ENGINE=direct/DMS=true 🔄​f ✅​p ❌​F ❌​F ❌​F ❌​F
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S

@shreyas-goenka
shreyas-goenka force-pushed the isaac/pr6052-fixes branch 2 times, most recently from 831045c to e9dbe85 Compare September 8, 2026 13:57
Base automatically changed from isaac/pr6052-fixes to main September 9, 2026 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants